uefi: fs: Add file times plumbing #138918

Ayush1325 · 2025-03-25T03:03:32Z

Add plumbing to allow conversions to and from UEFI Time to Rust SystemTime.
Also add FileTimes implementation.

petrochenkov · 2025-03-25T15:12:53Z

(I'll initially mark these PRs as waiting on author in the sense that, that they are waiting for the author to summon @nicholasbishop and get a review. It can be marked as ready when there's a review.)
@rustbot author

library/std/src/sys/pal/uefi/time.rs

Ayush1325 · 2025-04-11T19:55:36Z

@rustbot label +O-UEFI

joboet

I'm a bit worried about the edge cases here. It's not that uncommon to end up with a system time of 1970-1-1 if the devices clock is uninitialised. If the current timezone is positive, this might lead to a panic when converting it to a UNIX time. Considering the insanely large bounds afforded by secs being 64-bit, using a signed integer in the conversion algorithm would allow converting dates before the UNIX epoch as well.

Aside from that, the checked methods on SystemTime operate on the assumption that the operation will fail if the time is not representable in the operating system format – but this is currently not the case: UEFI allows times from the year 1900 up to and including the year 9999, whereas a Duration since the UNIX epoch allows representing times from 1970 up to the heat death of the universe, but not before that. I think it would be better to use the UEFI time representation for SystemTime and only convert it into a Duration for the addition/subtraction operations.

joboet · 2025-04-13T14:03:11Z

library/std/src/sys/pal/uefi/time.rs

+        let secs = if timezone == r_efi::efi::UNSPECIFIED_TIMEZONE {
+            dur.as_secs()
+        } else {
+            dur.as_secs().checked_add_signed(-timezone as i64).unwrap()


I'd use subtraction here, just like the formula in the UEFI spec.

Suggested change

dur.as_secs().checked_add_signed(-timezone as i64).unwrap()

dur.as_secs().checked_sub_signed(timezone as i64).expect("times should be representable as local UEFI times")

checked_sub_signed is currently unstable. I can add the feature if that is fine though.

joboet · 2025-04-13T14:03:49Z

library/std/src/sys/pal/uefi/time.rs

+        let remaining_secs = secs % SECS_IN_DAY;
+        let z = days + 719468;
+        let era = z / 146097;
+        let doe = z - (era * 146097);


This is clear by operator precedence, and the source doesn't use parenthesis here either.

Suggested change

let doe = z - (era * 146097);

let doe = z - era * 146097;

Ayush1325 · 2025-04-13T14:31:39Z

I'm a bit worried about the edge cases here. It's not that uncommon to end up with a system time of 1970-1-1 if the devices clock is uninitialised. If the current timezone is positive, this might lead to a panic when converting it to a UNIX time. Considering the insanely large bounds afforded by secs being 64-bit, using a signed integer in the conversion algorithm would allow converting dates before the UNIX epoch as well.

Aside from that, the checked methods on SystemTime operate on the assumption that the operation will fail if the time is not representable in the operating system format – but this is currently not the case: UEFI allows times from the year 1900 up to and including the year 9999, whereas a Duration since the UNIX epoch allows representing times from 1970 up to the heat death of the universe, but not before that. I think it would be better to use the UEFI time representation for SystemTime and only convert it into a Duration for the addition/subtraction operations.

Yes, I did come to the same conclusion. I think the reason I initially went with duration was to have a simple implementation for all the function implemented on SystemTime (since all the results there are in Duration). I will try representing SystemTime using UEFI Time internally and see how it looks.

bors · 2025-08-12T16:39:00Z

☔ The latest upstream changes (presumably #145300) made this pull request unmergeable. Please resolve the merge conflicts.

Ayush1325 · 2025-08-12T18:25:33Z

@rustbot ready

ping @nicholasbishop @joboet

petrochenkov · 2025-08-12T18:29:37Z

r? @joboet

nicholasbishop · 2025-08-12T18:36:00Z

library/std/src/sys/fs/uefi.rs


-#[derive(Copy, Clone, Debug, Default)]
-pub struct FileTimes {}
+#[derive(Copy, Clone, Debug)]


Time implements Default, so can't we keep deriving Default here and remove ZERO_TIME?

joboet · 2025-08-13T14:44:21Z

library/std/src/sys/fs/uefi.rs

-    pub fn set_modified(&mut self, _t: SystemTime) {}
+    pub fn set_accessed(&mut self, t: SystemTime) {
+        self.accessed =
+            t.to_uefi(self.accessed.timezone, self.accessed.daylight).expect("Invalid Time");


Can we avoid this panic? As the SystemTime was created successfully (i.e. checked_add did not return an error), this panic will come as a surprise to users.

A better way perhaps would be to try the initial timezone first, and if that fails, return the closest timezone (minute offset) that still allows the time to be represented. If filesystems ignore the timezone field, this would lead to the first and last 1440 minutes being collapsed into one minute, which doesn't seem too bad.

I have added the appropriate helpers to use the closest timezone. I have also added the tests that I have used locally to check the new timezone.

nicholasbishop

uefi lgtm

View changes since this review

nicholasbishop · 2025-08-26T16:29:54Z

library/std/src/sys/pal/uefi/time.rs

        system_time_internal::to_uefi(&self.0, timezone, daylight)
    }

+    /// Create UEFI Time with the closes timezone (minute offset) that still allows the time to be


Suggested change

/// Create UEFI Time with the closes timezone (minute offset) that still allows the time to be

/// Create UEFI Time with the closest timezone (minute offset) that still allows the time to be

beetrees

Thanks for working on this.

View changes since this review

beetrees · 2025-08-26T21:35:27Z

library/std/src/sys/fs/uefi.rs

 pub struct FileAttr {
    attr: u64,
    size: u64,
+    created: r_efi::efi::Time,


UEFI allows creation time to be set, so created should be part of the FileTimes struct (a future PR can then use it to implement set_created in an extension trait like on Windows and Apple platforms).

beetrees · 2025-08-26T21:51:23Z

library/std/src/sys/fs/uefi.rs

-    pub fn set_accessed(&mut self, _t: SystemTime) {}
-    pub fn set_modified(&mut self, _t: SystemTime) {}
+    pub fn set_accessed(&mut self, t: SystemTime) {
+        self.accessed = t.to_uefi_loose(self.accessed.timezone, self.accessed.daylight);


.timezone and .daylight will always be zero here (unless set_accessed/set_modified was called previously and that time had to have it's timezone adjusted to fit in the range of EFI_TIME). The EDK II FAT driver will ignore the timezone when setting the time.. As FAT stores timestamps in the system local time, this conversion should use the current system timezone from the runtime services GetTime in order to be compatible with that driver's behaviour. This conversion should be done in the File::set_times implementation to have consistent behaviour in the (unlikely) event the timezone is changed between the call to set_accessed and the actual setting of the file times in File::set_times (which I believe isn't being implemented in this PR); therefore the FileTimes struct should look something like struct FileTimes { accessed: SystemTime, modified: SystemTime, created: SystemTime }.

I have converted all FileTimes members to be SystemTime.

beetrees · 2025-08-26T21:51:27Z

library/std/src/sys/fs/uefi.rs


    pub fn modified(&self) -> io::Result<SystemTime> {
-        unsupported()
+        Ok(SystemTime::from_uefi(self.times.modified))


FAT timestamps are always stored in local time in the system timezone, and the EDK II FAT driver uses EFI_UNSPECIFIED_TIMEZONE to represent this. Therefore, if the modified/accessed/created EFI time has a timezone of unspecified, this conversion should use the timezone from the runtime services GetTime in order to be compatible.

Since this PR does not have any boundary code, I have added helper uefi_to_systemtime that uses the timezone from current time in unspecified timezone cases.
When converting back to UEFI time, I am also using local timezone now in the helper systemtime_to_uefi

All these helpers are local fs since we do not know if some other subsystem might want to handle these specific cases differently.

joboet · 2025-09-26T15:41:29Z

library/std/src/sys/fs/uefi.rs

 pub struct FileAttr {
    attr: u64,
    size: u64,
+    times: FileTimes,


If I understand correctly, UEFI's EFI_FILE_INFO always contains the file times, right? Then this shouldn't be FileTimes, but rather unconditionally contain the SystemTimes. That also gets rid of the weird unsupported error in the accessors below.

joboet · 2025-09-30T15:58:42Z

library/std/src/sys/pal/uefi/tests.rs

    assert_eq!(from_uefi(&t), Duration::new(1440 * SECS_IN_MINUTE, 0));
    assert_eq!(t, to_uefi(&from_uefi(&t), 0, 0).unwrap());
-    assert!(to_uefi(&from_uefi(&t), -1440, 0).is_some());
+    assert!(to_uefi(&from_uefi(&t), -1440, 0).is_err());


Something is wrong in the code, this should succeed (localtime = UTC - timezone, so the local time is after the 1st of january 1900).

My bad, ran it locally and it does succeed. So the test was wrong instead of the implementation.

joboet · 2025-09-30T16:01:01Z

library/std/src/sys/pal/uefi/tests.rs

+
+    let inp = Duration::from_secs(1450 * SECS_IN_MINUTE + 10);
+    let new_tz = to_uefi(&inp, 1440, 0).err().unwrap();
+    assert_eq!(new_tz, 9);


This is incorrect, if 1450 * SECS_IN_MINUTE is representable in timezone 10, then times after that should be too, right?

Again, ran it locally, and yes, the testcase currently fails. It should be 10. So the test was wrong instead of the implementation.

joboet · 2025-09-30T16:03:02Z

library/std/src/sys/pal/uefi/time.rs

    const SECS_IN_HOUR: u64 = SECS_IN_MINUTE * 60;
    const SECS_IN_DAY: u64 = SECS_IN_HOUR * 24;
-    const TIMEZONE_DELTA: u64 = 1440 * SECS_IN_MINUTE;
+    const SYSTEMTIME_TIMEZONE: u64 = 1440 * SECS_IN_MINUTE;


The timezone for SystemTime is -1440, since that is the timezone with the earliest representable value. So the sign of this value should be flipped (as well as the sign of the additions/subtractions you use it for).

joboet · 2025-09-30T16:05:16Z

library/std/src/sys/pal/uefi/time.rs

+        let adjusted_localtime_epoc: u64 = localtime_epoch + SYSTEMTIME_TIMEZONE;

        let epoch: u64 = if t.timezone == r_efi::efi::UNSPECIFIED_TIMEZONE {
            adjusted_localtime_epoc
        } else {
            adjusted_localtime_epoc
                .checked_add_signed((t.timezone as i64) * SECS_IN_MINUTE as i64)
                .unwrap()
        };


Splitting this calculation is confusing to me, as we want to do a conversion from the original timezone to timezone -1440. Could you perhaps normalise the timezone first (change UNSPECIFIED_TIMEZONE to UTC) and then do the conversion in one go?

joboet · 2025-09-30T16:07:35Z

library/std/src/sys/pal/uefi/time.rs


        // FIXME(#126043): use checked_sub_signed once stabilized
        let secs =
            dur.as_secs().checked_add_signed((-timezone as i64) * SECS_IN_MINUTE as i64).unwrap();


This will still panic for Duration::ZERO in e.g. timezone 1440 though.

bors · 2025-10-22T16:49:10Z

☔ The latest upstream changes (presumably #147826) made this pull request unmergeable. Please resolve the merge conflicts.

- Add FileTimes implementation. Signed-off-by: Ayush Singh <[email protected]>

rustbot · 2025-11-10T15:09:41Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Ayush1325 · 2025-11-10T15:13:29Z

@rustbot ready

joboet

Apart from the question, this looks great! Thank you for your patience.

View changes since this review

joboet · 2025-11-13T19:33:06Z

library/std/src/sys/pal/uefi/tests.rs

+
+#[test]
+fn max_time() {
+    let inp = MAX_UEFI_TIME.0;


Does this compile? MAX_UEFI_TIME is a Duration, isn't it?

Yes, it does. MAX_UEFI_TIME is SystemTime. So 0 field is the actual duration.

joboet · 2025-11-13T19:42:23Z

r=me after answering
@bors delegate+

bors · 2025-11-13T19:42:27Z

✌️ @Ayush1325, you can now approve this pull request!

If @joboet told you to "r=me" after making some further change, please make that change, then do @bors r=@joboet

Ayush1325 · 2025-11-14T13:49:06Z

@bors r=@joboet

bors · 2025-11-14T13:49:09Z

📌 Commit 1f0ad37 has been approved by joboet

It is now in the queue for this repository.

bors · 2025-11-14T15:35:43Z

⌛ Testing commit 1f0ad37 with merge 42f4793...

bors · 2025-11-14T18:46:33Z

☀️ Test successful - checks-actions
Approved by: joboet
Pushing 42f4793 to main...

github-actions · 2025-11-14T18:49:13Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing c910098 (parent) -> 42f4793 (this PR)

Test differences

Show 2 test diffs

2 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 42f4793e5a514858221b07ac379029d90353913e --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

dist-x86_64-apple: 6396.7s -> 8579.0s (+34.1%)
aarch64-apple: 10662.9s -> 8240.6s (-22.7%)
dist-arm-linux-musl: 5576.4s -> 6369.7s (+14.2%)
dist-aarch64-apple: 7794.6s -> 8637.6s (+10.8%)
x86_64-gnu-nopt: 7373.5s -> 8088.2s (+9.7%)
aarch64-gnu: 6327.3s -> 6909.7s (+9.2%)
x86_64-gnu-gcc: 3111.9s -> 2826.2s (-9.2%)
dist-aarch64-msvc: 5465.1s -> 5940.5s (+8.7%)
i686-msvc-1: 10747.7s -> 9907.1s (-7.8%)
dist-powerpc64le-linux-musl: 5373.6s -> 4954.2s (-7.8%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-11-14T20:06:52Z

Finished benchmarking commit (42f4793): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (secondary -2.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 472.486s -> 473.087s (0.13%)
Artifact size: 388.72 MiB -> 388.70 MiB (-0.01%)

rustbot assigned petrochenkov Mar 25, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 25, 2025

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 25, 2025

beetrees reviewed Mar 25, 2025

View reviewed changes

library/std/src/sys/pal/uefi/time.rs Outdated Show resolved Hide resolved

Ayush1325 force-pushed the uefi-fs-time branch from 394434c to 4c497b4 Compare March 30, 2025 17:11

Ayush1325 mentioned this pull request Apr 2, 2025

Tracking Issue for uefi-std #100499

Open

rustbot added the O-UEFI UEFI label Apr 11, 2025

joboet reviewed Apr 13, 2025

View reviewed changes

Ayush1325 force-pushed the uefi-fs-time branch from 4c497b4 to 48330eb Compare August 12, 2025 16:43

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 12, 2025

rustbot assigned joboet and unassigned petrochenkov Aug 12, 2025

nicholasbishop reviewed Aug 12, 2025

View reviewed changes

Ayush1325 force-pushed the uefi-fs-time branch from 48330eb to e21995b Compare August 12, 2025 18:38

joboet reviewed Aug 13, 2025

View reviewed changes

Ayush1325 force-pushed the uefi-fs-time branch from e21995b to 6637ec8 Compare August 13, 2025 16:49

Ayush1325 requested review from joboet and nicholasbishop August 16, 2025 06:36

nicholasbishop approved these changes Aug 26, 2025

View reviewed changes

Ayush1325 force-pushed the uefi-fs-time branch from 6637ec8 to 0632fd5 Compare August 26, 2025 17:24

beetrees suggested changes Aug 26, 2025

View reviewed changes

Ayush1325 force-pushed the uefi-fs-time branch from 9425040 to 0629cda Compare September 26, 2025 15:23

This comment has been minimized.

Sign in to view

Ayush1325 requested a review from joboet September 26, 2025 15:24

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 26, 2025

joboet reviewed Sep 30, 2025

View reviewed changes

joboet added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2025

uefi: fs: Add file times plumbing

1f0ad37

- Add FileTimes implementation. Signed-off-by: Ayush Singh <[email protected]>

Ayush1325 force-pushed the uefi-fs-time branch from 0629cda to 1f0ad37 Compare November 10, 2025 15:09

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 10, 2025

joboet reviewed Nov 13, 2025

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 14, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 14, 2025

bors merged commit 42f4793 into rust-lang:main Nov 14, 2025
12 checks passed

rustbot added this to the 1.93.0 milestone Nov 14, 2025

Ayush1325 deleted the uefi-fs-time branch November 15, 2025 03:52

	dur.as_secs().checked_add_signed(-timezone as i64).unwrap()
	dur.as_secs().checked_sub_signed(timezone as i64).expect("times should be representable as local UEFI times")

	/// Create UEFI Time with the closes timezone (minute offset) that still allows the time to be
	/// Create UEFI Time with the closest timezone (minute offset) that still allows the time to be

uefi: fs: Add file times plumbing #138918

uefi: fs: Add file times plumbing #138918

Conversation

Ayush1325 commented Mar 25, 2025

Uh oh!

petrochenkov commented Mar 25, 2025

Uh oh!

Uh oh!

Ayush1325 commented Apr 11, 2025

Uh oh!

joboet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ayush1325 commented Apr 13, 2025

Uh oh!

bors commented Aug 12, 2025

Uh oh!

Ayush1325 commented Aug 12, 2025

Uh oh!

petrochenkov commented Aug 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicholasbishop left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beetrees left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicholasbishop left a comment •

edited by rustbot

Loading

beetrees left a comment •

edited by rustbot

Loading

joboet left a comment •

edited by rustbot

Loading